Skip to content

baggage: Accept non-ASCII keys #5132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 35 commits into from
Aug 15, 2024
Merged

baggage: Accept non-ASCII keys #5132

merged 35 commits into from
Aug 15, 2024

Conversation

@XSAM
Copy link
Member

@XSAM XSAM commented Apr 2, 2024 •
edited
Loading

resolves #4946

I also add additional test cases to cover more lines.

benchmark results:

goos: darwin
goarch: arm64
pkg: go.opentelemetry.io/otel/baggage
                                 │   old.txt   │               new.txt               │
                                 │   sec/op    │   sec/op     vs base                │
New-10                             402.3n ± 1%   422.4n ± 6%   +4.98% (p=0.000 n=10)
NewMemberRaw-10                    10.82n ± 0%   13.90n ± 1%  +28.51% (p=0.000 n=10)
Parse-10                           803.8n ± 1%   795.0n ± 1%   -1.09% (p=0.011 n=10)
String-10                          682.6n ± 0%   610.0n ± 2%  -10.63% (p=0.000 n=10)
ValueEscape/nothing_to_escape-10   4.856n ± 0%   4.849n ± 0%        ~ (p=0.279 n=10)
ValueEscape/requires_escaping-10   22.47n ± 1%   22.36n ± 1%        ~ (p=0.342 n=10)
ValueEscape/long_value-10          513.3n ± 1%   510.1n ± 0%   -0.62% (p=0.006 n=10)
MemberString-10                    430.8n ± 2%   471.3n ± 2%   +9.41% (p=0.000 n=10)
geomean                            124.5n        128.5n        +3.22%

                                 │   old.txt    │               new.txt                │
                                 │     B/op     │    B/op     vs base                  │
New-10                             704.0 ± 0%     704.0 ± 0%        ~ (p=1.000 n=10) ¹
NewMemberRaw-10                    0.000 ± 0%     0.000 ± 0%        ~ (p=1.000 n=10) ¹
Parse-10                           888.0 ± 0%     888.0 ± 0%        ~ (p=1.000 n=10) ¹
String-10                          936.0 ± 0%     840.0 ± 0%  -10.26% (p=0.000 n=10)
ValueEscape/nothing_to_escape-10   0.000 ± 0%     0.000 ± 0%        ~ (p=1.000 n=10) ¹
ValueEscape/requires_escaping-10   16.00 ± 0%     16.00 ± 0%        ~ (p=1.000 n=10) ¹
ValueEscape/long_value-10          576.0 ± 0%     576.0 ± 0%        ~ (p=1.000 n=10) ¹
MemberString-10                    656.0 ± 0%     656.0 ± 0%        ~ (p=1.000 n=10) ¹
geomean                                       ²                -1.34%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                                 │    old.txt    │               new.txt                │
                                 │   allocs/op   │ allocs/op   vs base                  │
New-10                              8.000 ± 0%     8.000 ± 0%        ~ (p=1.000 n=10) ¹
NewMemberRaw-10                     0.000 ± 0%     0.000 ± 0%        ~ (p=1.000 n=10) ¹
Parse-10                            9.000 ± 0%     9.000 ± 0%        ~ (p=1.000 n=10) ¹
String-10                          10.000 ± 0%     8.000 ± 0%  -20.00% (p=0.000 n=10)
ValueEscape/nothing_to_escape-10    0.000 ± 0%     0.000 ± 0%        ~ (p=1.000 n=10) ¹
ValueEscape/requires_escaping-10    1.000 ± 0%     1.000 ± 0%        ~ (p=1.000 n=10) ¹
ValueEscape/long_value-10           2.000 ± 0%     2.000 ± 0%        ~ (p=1.000 n=10) ¹
MemberString-10                     4.000 ± 0%     4.000 ± 0%        ~ (p=1.000 n=10) ¹
geomean                                        ²                -2.75%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

FYI, the old implementation of NewMemberRaw didn't verify the value, so the benchmark result of NewMemberRaw is not an apple-to-apple comparison of utf8.ValidString and validateKey.

@codecov
Copy link

codecov bot commented Apr 2, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.5%. Comparing base (1dc9522) to head (9559edd).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5132   +/-   ##
=====================================
  Coverage   84.4%   84.5%           
=====================================
  Files        272     272           
  Lines      22519   22554   +35     
=====================================
+ Hits       19028   19067   +39     
+ Misses      3155    3153    -2     
+ Partials     336     334    -2

see 2 files with indirect coverage changes

@XSAM XSAM marked this pull request as ready for review April 2, 2024 05:52
pellared
pellared previously requested changes Apr 2, 2024
View reviewed changes
@pellared pellared mentioned this pull request Apr 2, 2024
Open
@pellared pellared added blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made area:baggage Part of OpenTelemetry baggage and removed blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made labels Apr 2, 2024
@pellared pellared dismissed their stale review April 3, 2024 11:00

Unblocking this PR

@XSAM XSAM requested a review from pellared April 21, 2024 22:31
@pellared pellared mentioned this pull request Jul 29, 2024
Closed
pellared
pellared reviewed Jul 29, 2024
View reviewed changes
@pellared pellared self-requested a review July 29, 2024 08:07
@pellared pellared dismissed their stale review July 29, 2024 08:08

After reflection I think that the changes may be more confusing for the users than helpful. https://github.com/open-telemetry/opentelemetry-go/pull/5132/files#r1694778279

pellared
pellared approved these changes Jul 31, 2024
View reviewed changes
pellared
pellared reviewed Aug 1, 2024
View reviewed changes
@MrAlias MrAlias added this to the v1.29.0 milestone Aug 8, 2024
@XSAM XSAM merged commit d61bbf1 into open-telemetry:main Aug 15, 2024
@XSAM XSAM deleted the utf8-baggage branch August 15, 2024 23:50
@MrAlias MrAlias mentioned this pull request Aug 23, 2024
Merged
@renovate renovate bot mentioned this pull request Aug 23, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dashpole dashpole dashpole approved these changes

@pellared pellared pellared approved these changes

@MrAlias MrAlias Awaiting requested review from MrAlias MrAlias is a code owner

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@evantorrie evantorrie Awaiting requested review from evantorrie

@MadVikingGod MadVikingGod Awaiting requested review from MadVikingGod

@hanyuancheung hanyuancheung Awaiting requested review from hanyuancheung

@dmathieu dmathieu Awaiting requested review from dmathieu dmathieu is a code owner

Assignees

No one assigned

Labels

area:baggage Part of OpenTelemetry baggage

Projects

None yet

Milestone

v1.29.0

Development

Successfully merging this pull request may close these issues.

baggage: Accept non-ASCII keys

5 participants

@XSAM @pellared @dashpole @MrAlias @hanyuancheung